Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(GH<->GGs): Repair GGs #996

Merged
merged 19 commits into from
Oct 27, 2023
Merged

Conversation

kishore03109
Copy link
Contributor

@kishore03109 kishore03109 commented Oct 26, 2023

Problem

Pains:

  1. Need for Ops to ask Eng to reset when there is a drift from GH and EFS
  2. If quickie has bugs that cause drift between staging and staging-lite, Eng would have had to go into efs, rm the staging-lite repo, git clone and make the repo gitless and assetless before committing to staging-lite to make sure that the repos are in sync
  3. Need for Eng to clone repos into EFS during email migration. (Phase 2 shipposting)
  4. Need for Eng to set up staging-lite for quickie (One time effort)
    This is also annoying since we need to go through a bunch of hops to enter into our efs in prod, which makes oncall a pain.

Rather than having separate scripts for each of this pain point, this form intends to kill 4 birds in one stone.

Solution

This formsg script is a idempotent script to be used when

  1. When we want to clone a repo into GGs.
  2. When we want to modify a repo directly in Github and we want to sync the repos.
  3. Repair possible drifts between staging and staging lite branches.

addressing @alexanderleegs' comments at the PR desc level cuz its not v clear from just the code

Hmm actually does this assume that a staging lite doesn't exist? it seems to attempt a clone + pull, but we also want this to be able to repair error right, is this the desired behaviour?

We always assume staging lite doesnt exist. the reason why we dont git pull is because once we git pull, .git reconciles and the folder bloats, and we lose the benefits of a small bundle size. so we always reset, and this is not too long (< 1min for nlb iirc.

We also always clone and pull. the reason for this is because cloning takes time, with repos like nlb it can take a while, so we do a pull right before we reset staging lite.

It is notable that during this period of time, any changes to the repo will screw this repair up, but actually this is ok because:

  1. we can always re-run this script
  2. we are not rate-limited in any way for the above ^

Breaking Changes

  • Yes - this PR contains breaking changes
    • flow has to change for phase 2 migration, updated in the read me post review
  • No - this PR is backwards compatible with ALL of the following feature flags in this doc

Screenshots

Error state:
Screenshot 2023-10-27 at 12 00 59 PM

Success state:
Screenshot 2023-10-27 at 12 01 05 PM

Tests

// staging ssm stuff is not working.

Deploy Notes

New environment variables:

  • GGS_REPAIR_FORM_KEY : Form secret key
    • added env var to 1PW + SSM script (fetch_ssm_parameters.sh)

New scripts:

  • script : script details

New dependencies:

  • dependency : dependency details

New dev dependencies:

  • dependency : dependency details

@kishore03109 kishore03109 changed the title IS-702-Enhance-repo-cloning-for-GGS feat(GH-GGs): Repair GGs Oct 26, 2023
@kishore03109 kishore03109 changed the title feat(GH-GGs): Repair GGs feat(GH<->GGs): Repair GGs Oct 26, 2023
@kishore03109 kishore03109 marked this pull request as ready for review October 27, 2023 04:19
@kishore03109 kishore03109 requested review from a team and alexanderleegs October 27, 2023 04:19
Copy link
Contributor

@alexanderleegs alexanderleegs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left comments!

Do also add the new env var to the deploy script to fetch ssm params

src/routes/formsg/formsgGGsRepair.ts Outdated Show resolved Hide resolved
src/routes/formsg/formsgGGsRepair.ts Outdated Show resolved Hide resolved
Comment on lines +154 to +158
fromPromise(
this.reposService.setUpStagingLite(
path.join(EFS_VOL_PATH_STAGING_LITE, repoName),
repoUrl
),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm actually does this assume that a staging lite doesn't exist? it seems to attempt a clone + pull, but we also want this to be able to repair error right, is this the desired behaviour?

Copy link
Contributor Author

@kishore03109 kishore03109 Oct 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually fair this not v clear, updated pr desc
We always assume staging lite doesnt exist. the reason why we dont git pull is because once we git pull, .git reconciles and the folder bloats, and we lose the benefits of a small bundle size. so we always reset, and this is not too long (< 1min for nlb iirc.

We also always clone and pull. the reason for this is because cloning takes time, with repos like nlb it can take a while, so we do a pull right before we reset staging lite.

It is notable that during this period of time, any changes to the repo will screw this repair up, but actually this is ok because:

  1. we can always re-run this script
  2. we are not rate-limited in any way for the above ^

): void => {
const submission = this.formsg.crypto.decrypt(
): Promise<void> => {
const submission = await this.formsg.crypto.decryptWithAttachments(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do existing forms (without attachments) work with this? do we need a conditional on attachment existence?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, checked already

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait to be clear this was a breaking change to the api, so the other forms had to change and I checked that the other forms were able to parse the content (ie the individual fields)

@kishore03109
Copy link
Contributor Author

Do also add the new env var to the deploy script to fetch ssm params
@alexanderleegs , yup added

Copy link
Contributor

@alexanderleegs alexanderleegs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@kishore03109 kishore03109 merged commit 2aa5160 into develop Oct 27, 2023
8 checks passed
@mergify mergify bot deleted the IS-702-Enhance-repo-cloning-for-GGS branch October 27, 2023 08:54
This was referenced Oct 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants